Silent disconnection guard bypass when slot lookup fails

When slotIndex remains -1 (either because getNthPlayer returns null, or because no network slot's getPlayerName() matches the player's display name), the condition slotIndex >= 0 && !isPlayerConnected(slotIndex) short-circuits to false, so the continue is never taken. That means a disconnected player whose name has already been cleared by the network layer (common on disconnect) will still have their stale cached CRC included in the comparison, potentially producing the same spurious mismatch that Caball009's original commit was trying to avoid.

The same pattern appears identically in GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp line 2688–2689.

If the intent is to treat "unresolvable slot" conservatively (always check), the current behaviour is defensible. But if getPlayerName() can return empty after disconnection, you may want to also continue when slotIndex == -1 — or at least add a comment explaining why proceeding is safe in that case.

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
Line: 2362-2363

Comment:
**Silent disconnection guard bypass when slot lookup fails**

When `slotIndex` remains `-1` (either because `getNthPlayer` returns null, or because no network slot's `getPlayerName()` matches the player's display name), the condition `slotIndex >= 0 && !isPlayerConnected(slotIndex)` short-circuits to `false`, so the `continue` is never taken. That means a disconnected player whose name has already been cleared by the network layer (common on disconnect) will still have their stale cached CRC included in the comparison, potentially producing the same spurious mismatch that Caball009's original commit was trying to avoid.

The same pattern appears identically in `GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp` line 2688–2689.

If the intent is to treat "unresolvable slot" conservatively (always check), the current behaviour is defensible. But if `getPlayerName()` can return empty after disconnection, you may want to also `continue` when `slotIndex == -1` — or at least add a comment explaining why proceeding is safe in that case.

How can I resolve this? If you propose a fix, please make it concise.

What about this comment?

The new @bugfix comment only explains the index mapping fix, but doesn't address what happens when slotIndex == -1 — which is the edge case my review flagged. A reader looking at:

if (slotIndex >= 0 && !TheNetwork->isPlayerConnected(slotIndex))
    continue;

has no way to know whether falling through when slotIndex == -1 is intentional ("always validate CRC if we can't resolve the slot") or an oversight ("might miss the disconnected-player guard").

The comment should be extended to make the intent explicit. For example:

Alternatively, if the intent is actually to skip unresolvable slots (matching the conservative spirit of Caball009's original fix), the guard should be:

if (slotIndex < 0 || !TheNetwork->isPlayerConnected(slotIndex))
    continue;

Which of those two behaviours is intended?

Any reason why the PlayerList class owns the slot indices as opposed to the GameInfo class owning the player indices?

I am not sure right now which one is better, but my first instinct would have been to tie it to GameInfo class, because that is closer to the slots. Do we have any opinions on this?

I think PlayerList is preferable because there are a couple of places where it's not clear to me if we could easily access GameInfo.

You can search the code base for player%d to find them; one example:

Byte is unsigned on ARM, making the -1 sentinel become 255

Byte is defined as typedef char Byte in BaseTypeCore.h. On ARM (Apple Silicon Mac — explicitly listed as a tested platform), char is unsigned by default, so return -1 returns unsigned char(255). When GameLogic.cpp stores this in const Int slotIndex, it zero-extends to 255 rather than sign-extending to -1. The guard slotIndex >= 0 then evaluates to true (255 ≥ 0), and isPlayerConnected(255) is called. ConnectionManager::isPlayerConnected directly indexes m_connections[playerID] (with a DEBUG_ASSERTCRASH for playerID < MAX_SLOTS): in debug builds this crashes the assertion, in release builds it is an out-of-bounds memory read on any slot lookup that failed to resolve.

The same applies to GeneralsMD/Code/GameEngine/Source/Common/RTS/PlayerList.cpp.

The return type (and array element type) should be Int to make the sentinel value unambiguous across platforms, or the array should use signed char explicitly.

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/Common/RTS/PlayerList.cpp
Line: 497-504

Comment:
**`Byte` is unsigned on ARM, making the -1 sentinel become 255**

`Byte` is defined as `typedef char Byte` in `BaseTypeCore.h`. On ARM (Apple Silicon Mac — explicitly listed as a tested platform), `char` is unsigned by default, so `return -1` returns `unsigned char(255)`. When `GameLogic.cpp` stores this in `const Int slotIndex`, it zero-extends to `255` rather than sign-extending to `-1`. The guard `slotIndex >= 0` then evaluates to `true` (255 ≥ 0), and `isPlayerConnected(255)` is called. `ConnectionManager::isPlayerConnected` directly indexes `m_connections[playerID]` (with a `DEBUG_ASSERTCRASH` for `playerID < MAX_SLOTS`): in debug builds this crashes the assertion, in release builds it is an out-of-bounds memory read on any slot lookup that failed to resolve.

The same applies to `GeneralsMD/Code/GameEngine/Source/Common/RTS/PlayerList.cpp`.

The return type (and array element type) should be `Int` to make the sentinel value unambiguous across platforms, or the array should use `signed char` explicitly.

How can I resolve this? If you propose a fix, please make it concise.

what about this greptile warning?

If this is an issue for ARM, that needs to be handled separately. There's no reason to change from Byte to Int just for this.

The technical risk is real and specific to this PR: Apple Silicon Mac is explicitly listed in the PR description as a tested platform, and getSlotIndex returning -1 as Byte will produce 255 on that platform. Since 255 >= 0 is true, the sentinel fails silently and isPlayerConnected(255) gets called — an out-of-bounds access.

That said, you're right that the fix doesn't have to be Int. A minimal in-scope fix would be to use signed char explicitly for the array and return type, which preserves the Byte-like size while making the signedness unambiguous across platforms. Either way, handling it in a follow-up PR is your call — just worth noting that the sentinel is broken on the very platform this bugfix was discovered on, so it'd be good to track.

This project doesn't target ARM, nor is it the responsibility of this PR to take this into account.

Agreed, can add it as a separate issue and PR, though in the interest of keeping reviewers focused on releasing 1.0, I'd set this part aside for a later date. I've also come across weird things where the original code would use weirdly wrapped numbers in my bgfx fork.

You can solve this simply by adding a bool PlayerList::hasSlotIndex(Int playerIndex) function. Then use that instead of comparing the returned index.

Or provide a InvalidSlotIndex = -1 value and compare against that.

I see you changed Byte to Int already, which is also ok.

The return type was indeed changed to Int in the current diff, which resolves the original ARM/unsigned concern — good catch there.

On xezon's suggestions:

• hasSlotIndex(playerIndex) would encapsulate the validity check inside PlayerList, which is slightly cleaner.
• InvalidSlotIndex = -1 constant would make the magic number self-documenting.

Both are reasonable style improvements, but the current slotIndex >= 0 check is readable and correct as written with Int. It's a matter of preference — the fix is sound either way.

Maybe call it assignSlotIndices ?

Maybe make GameInfo an argument

assignSlotIndices(const GameInfo& gameInfo)

It makes the intent of the function even clearer from the outside.

if (TheGameInfo)
  assignSlotIndices(TheGameInfo)

You can solve this simply by adding a bool PlayerList::hasSlotIndex(Int playerIndex) function. Then use that instead of comparing the returned index.

Or provide a InvalidSlotIndex = -1 value and compare against that.

I see you changed Byte to Int already, which is also ok.

I can confirm that this is called after newGame, overwriting its assignments.

Is it necessary to do anything in loadPostProcess?